[InstCombine] Improve foldDeadPhiWeb compile time
#158057
Conversation
The foldDeadPhiWeb function identifies and removes small dead PHI webs, it bails out if the web size exceeds threshold (16). In the current implementation, when there is a phi node has large number of users that most of them are phi nodes, we still push them on the `Stack` even if the numbers of phi nodes user already exceed the threshold. It is unnecessary and may consume too much time. This patch checks the early stop condition when we push an unvisited phi node on the `Stack`. With this change, the wall duration of total instcombine pass decreased from 523,649.276 ms to 30,239.399 ms in an our internal case.
llvmbot
added
llvm:instcombine
|
@llvm/pr-subscribers-llvm-transforms Author: Mingjie Xu (Enna1) ChangesThe foldDeadPhiWeb function identifies and removes small dead PHI webs, it bails out if the web size exceeds threshold (16). In the current implementation, when there is a phi node has large number of users that most of them are phi nodes, we still push them on the This patch checks the early stop condition when we push an unvisited phi node on the Full diff: https://github.com/llvm/llvm-project/pull/158057.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
index ed9a0be6981fa..c3643a7cb4c08 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
@@ -62,14 +62,15 @@ bool InstCombinerImpl::foldDeadPhiWeb(PHINode &PN) {
Stack.push_back(&PN);
while (!Stack.empty()) {
PHINode *Phi = Stack.pop_back_val();
- if (!Visited.insert(Phi).second)
- continue;
- // Early stop if the set of PHIs is large
- if (Visited.size() == 16)
- return false;
for (User *Use : Phi->users()) {
- if (PHINode *PhiUse = dyn_cast<PHINode>(Use))
+ if (PHINode *PhiUse = dyn_cast<PHINode>(Use)) {
+ if (!Visited.insert(Phi).second)
+ continue;
+ // Early stop if the set of PHIs is large
+ if (Visited.size() >= 16)
+ return false;
Stack.push_back(PhiUse);
+ }
else
return false;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Clang fails to bootstrap with this change: https://llvm-compile-time-tracker.com/show_error.php?commit=bc97a194ac4576b4bbe448f73314f746d3411a0d |
| for (User *Use : Phi->users()) { | ||
| if (PHINode *PhiUse = dyn_cast<PHINode>(Use)) | ||
| if (PHINode *PhiUse = dyn_cast<PHINode>(Use)) { | ||
| if (!Visited.insert(Phi).second) |
There was a problem hiding this comment.
This should insert PhiUse, not Phi.
| bool InstCombinerImpl::foldDeadPhiWeb(PHINode &PN) { | ||
| SmallVector<PHINode *, 16> Stack; | ||
| SmallPtrSet<PHINode *, 16> Visited; | ||
| Stack.push_back(&PN); |
There was a problem hiding this comment.
We should also add the initial phi node to the Visited set.
There was a problem hiding this comment.
LGTM. Not seeing any impact on average cases (https://llvm-compile-time-tracker.com/compare.php?from=e790c97f65687bece901072cb4f6935061785536&to=63bfce66db9ade00b3140e637d83ca41e48909bc&stat=instructions:u), but will help degenerate ones.
|
Thanks for your kind review. |
3 participants
The foldDeadPhiWeb function identifies and removes small dead PHI webs, it bails out if the web size exceeds threshold (16).
In the current implementation, when there is a phi node has large number of users that most of them are phi nodes, we still push them on the
Stackeven if the number of phi nodes user exceeds the threshold.This patch checks the early stop condition when we push an unvisited phi node on the
Stack.With this change, the wall duration of total instcombine pass decreased from 523,649.276 ms to 208,687.042 ms in an our internal case.